Send an idempotency key on write requests - #17
Conversation
|
thanks! |
| _MAX_RETRIES = 3 | ||
| _RETRY_DELAYS = [1, 2, 4] # Exponential backoff: 1s, 2s, 4s | ||
|
|
||
| # The API replays the original response for a repeated Idempotency-Key instead of |
There was a problem hiding this comment.
Missing _RETRYABLE_STATUS_CODES constant: The refactored _is_retryable() function at line 36 references _RETRYABLE_STATUS_CODES but it is never defined in the diff. The code will raise NameError at runtime when a non-409 retryable status (502, 503, 429) is encountered. Define the constant before line 20.
| # The API replays the original response for a repeated Idempotency-Key instead of | |
| # The API replays the original response for a repeated Idempotency-Key instead of | |
| # running the operation again, so a retried write cannot create a duplicate record. | |
| # https://docs.dualentry.com/developers/release-notes/2026-08-12 | |
| _IDEMPOTENCY_HEADER = "Idempotency-Key" | |
| _IDEMPOTENCY_METHODS = frozenset({"POST", "PUT", "PATCH", "DELETE"}) | |
| # 429 and the in-flight 409 both report exactly how long to wait. | |
| # https://docs.dualentry.com/developers/guides/rate-limiting | |
| _RETRY_AFTER_HEADER = "Retry-After" | |
| _RETRYABLE_STATUS_CODES = frozenset({502, 503, 429}) |
There was a problem hiding this comment.
The mentioned constant was already in place, so it wasn't included into the PR.
| response = self._client.request(method, path, **kwargs) | ||
| return self._handle_response(response) | ||
|
|
||
| # Retry logic with visible feedback |
There was a problem hiding this comment.
Off-by-one in retry loop: The loop at line 148 runs _MAX_RETRIES times (3 iterations: attempts 0, 1, 2), then line 165 unconditionally issues a 4th request after the loop exits. This produces 4 total attempts instead of the advertised _MAX_RETRIES=3. Additionally, the stderr message at line 163 prints attempt + 2 and _MAX_RETRIES + 1 (producing "attempt 2/4"), but the final 4th request after the loop has no message. The user sees "Retrying" three times then a silent 4th attempt. Fix: move the final request inside the loop and return after each successful response; remove the unconditional request after line 164.
| # Retry logic with visible feedback | |
| # Retry logic with visible feedback | |
| last_error = None | |
| for attempt in range(_MAX_RETRIES): | |
| retry_after = None | |
| try: | |
| response = self._client.request(method, path, **kwargs) | |
| if not _is_retryable(response): | |
| return self._handle_response(response) | |
| retry_after = _retry_after_seconds(response) | |
| # Retryable error - will retry | |
| last_error = APIError(response.status_code, f"Temporary error ({response.status_code})") | |
| except httpx.RequestError as e: | |
| last_error = e | |
| if attempt < _MAX_RETRIES - 1: | |
| delay = retry_after if retry_after is not None else _RETRY_DELAYS[attempt] | |
| print(f"\033[33mRetrying in {delay:g}s... (attempt {attempt + 2}/{_MAX_RETRIES})\033[0m", file=sys.stderr) | |
| time.sleep(delay) | |
| # Final attempt | |
| response = self._client.request(method, path, **kwargs) | |
| return self._handle_response(response) |
| # every retry waits, including the one after the loop | ||
| delay = retry_after if retry_after is not None else _RETRY_DELAYS[attempt] | ||
| print(f"\033[33mRetrying in {delay:g}s... (attempt {attempt + 2}/{_MAX_RETRIES + 1})\033[0m", file=sys.stderr) | ||
| time.sleep(delay) |
There was a problem hiding this comment.
Inconsistent retry messaging and logic: Line 163 prints "attempt {attempt + 2}/{_MAX_RETRIES + 1}" (printing 2/4), but this message is shown only when attempt < _MAX_RETRIES - 1 is true (line 162). After the loop exits (all 3 iterations done), line 165 issues the 4th request without printing a message or waiting. The message at line 163 should print "{attempt + 2}/{_MAX_RETRIES}" to match the fixed loop logic; see prior comment.
There was a problem hiding this comment.
From the comment below I concluded that the 4th attempt was intentional, so left it unchanged and just fixed delay to honor the Retry-After header and the exponential backoff value used by default. Honestly, we can simply remove the "Final attempt" block and stay with only 3 attempts to retry, or increase the value of _MAX_RETRIES to 4 (I will do this). This would keep current behavior and make the code a bit cleaner.
There was a problem hiding this comment.
I tried to do it the way I wrote above, but in the end it made the code more complex instead of cleaner, so I left _MAX_RETRIES = 3 and the "Final attempt" block as they are.
One more thing I changed in this method is what exactly we catch. It was except httpx.RequestError, which is basically everything, including errors that can never succeed on a second attempt: a wrong scheme in the URL (UnsupportedProtocol), LocalProtocolError, DecodingError, TooManyRedirects. So if somebody sets a wrong DUALENTRY_API_URL, the CLI was sending 4 requests and sleeping 1+2+4 seconds before showing an error that was already known after the first one. Now we retry only timeouts, network errors and RemoteProtocolError, and everything else is reported immediately. Both lists are covered with tests.
2 participants
Summary
The
--retryflag repeats failed requests. It repeats all of them, includingPOST,PUT,PATCHandDELETE.The problem is that when a request fails with 502 or 504, we do not know what happened on the server. The request may have failed before anything was saved. Or it may have been processed correctly and only the response was lost. We cannot tell the difference from the client side.
If we retry in the second case, we create a second record. For an accounting CLI this means a duplicated invoice or journal entry.
The DualEntry API added idempotency keys on 2026-08-12:
This PR uses that header. Now every write request sends a key. All retries of the same request send the same key. The server then replays the first response instead of doing the work again, so we no longer need to know what happened on the server.
What the API promises
From the endpoint docs, for example create recurring request, update recurring request, delete recurring request and partial update of a customer payment:
The last point is important. Each logical request must get its own key. We cannot reuse one key for several requests.
The idempotency guide adds two more cases. Both return 409, but they mean opposite things:
Retry-AfterThe guide doesn't explain the type of value sent in the
Retry-Afterheader in this case. However, the rate limiting guide says about 429: "The response includes aRetry-Afterheader (seconds) telling you exactly how long to wait before the bucket refills enough for one more request." So I assumed that theRetry-Afterheader in case of a 409 error also contains the amount of seconds.Changes
In
src/dualentry_cli/client.py:_requestnow adds anIdempotency-Keyheader forPOST,PUT,PATCHandDELETE.GETdoes not get the header. It does not change anything on the server, so the header has no meaning there.uuid.uuid4(). It is created once per_requestcall, before the retry loop. This is the important part. If we created a new key for each attempt, the bug would still be there.--retryis off. Something else may repeat the request, for example a proxy. We usesetdefault, so if a caller passes its own key, we keep it.patch()method. The API documentsPATCHfor partial updates, but the client could not send one._is_retryable()separates the two 409 cases. A 409 withRetry-Afteris retried. A 409 without it is not retried at all._retry_after_seconds()reads the header. When it is present, the client waits exactly that long.2/3,3/3for four requests; now it says2/4,3/4,4/4.httpx.RequestError, which is every transport failure, including ones that fail the same way every time: a wrong scheme inDUALENTRY_API_URL(UnsupportedProtocol),LocalProtocolError,DecodingError,TooManyRedirectsandProxyError. A typo in the URL meant 4 requests and 1 + 2 + 4 seconds of waiting before showing an error that was already known after the first one. The new_RETRYABLE_EXCEPTIONSkeeps timeouts, network errors andRemoteProtocolError; everything else is reported immediately.last_errorvariable is gone. It was never read, and after the changes above there was nothing left for it to do.Tests
tests/test_client.pyhad no tests for the retry logic at all. A new classTestIdempotencyKeywith 10 cases was added:test_write_methods_send_an_idempotency_keytest_get_does_not_send_an_idempotency_keytest_retry_reuses_the_same_key_across_attemptstest_every_retry_attempt_carries_the_keytest_separate_requests_use_different_keystest_caller_supplied_key_is_not_overwrittentest_key_is_sent_even_when_retry_is_disabledretry=FalseSecond class
TestRetryAfterAndConflictswith 9 cases (27 including parameters):test_conflict_with_retry_after_is_retriedRetry-After: 2, then 201. Waits exactly 2s, reuses the keytest_conflict_without_retry_after_is_not_retriedtest_rate_limit_waits_for_retry_after_not_the_hardcoded_backoffRetry-After: 7waits 7s, not the 1s from_RETRY_DELAYStest_rate_limit_without_retry_after_falls_back_to_backofftest_the_last_attempt_also_waits_for_retry_afterRetry-After: 3. The waits are 3s, 3s, 3s, so the request after the loop waits tootest_unparsable_retry_after_falls_back_to_backoffRetry-After:next tuesday,inf,Infinity,1e9,2.5,-5, empty: all fall backtest_transient_transport_error_is_retriedRemoteProtocolError: 4 requests, waits 1s, 2s, 4s, then the error propagatestest_non_transient_transport_error_is_not_retriedLocalProtocolError,UnsupportedProtocol,ProxyError,DecodingError,TooManyRedirects: one request, no waitingtest_storage_unavailable_is_retried_with_the_same_keyTwo fixtures keep the tests fast and precise:
no_backoffsets_RETRY_DELAYSto zeros, otherwise the tests would really wait 1s, 2s and 4ssleepsreplacestimein the client module and records the waits, so a test can check the exact number of seconds instead of measuring real timeNote about the changes
The PR [#16] needs to be merged into
mainfirst, and those changes need to be pulled into this branch for the tests to pass. The changes were originally made and tested on top of the fix/ci-dependency-drift branch, then moved to the current branch, which was created frommain.Test plan
uv run pytest)uv run ruff check .)dualentry <command>(since I do not have a valid API key, I tested in a mocked environment)